[Proposed] ACP-99: Implement using composition - ValidatorManager as single entry point#668
[Proposed] ACP-99: Implement using composition - ValidatorManager as single entry point#668cam-schultz wants to merge 8 commits into
Conversation
I'm curious to know how much this affects gas costs, that would be one of the major advantages of keeping everything in a single contract, right? |
| abstract contract ACP99ValidatorManager is IACP99ValidatorManager, ValidatorManager { | ||
| IACP99SecurityModule public securityModule; | ||
|
|
||
| // TODO: calling this should be restricted to...who? |
There was a problem hiding this comment.
😜
| // TODO: calling this should be restricted to...who? | |
| // TODO: calling this should be restricted to...whom? |
We won't know until we get unit tests working, but in theory the only added cost should be the external call to the security module. If we're not transferring ETH, and assuming the address is "warm" (it should be - every interaction will call the same security module), then gas is on the order of hundreds https://github.com/wolflo/evm-opcodes/blob/main/gas.md#aa-1-call |
richardpringle
left a comment
richardpringle
left a comment
There was a problem hiding this comment.
What's the key difference here, that the security module is a property of the validator-manager instead of the validator-manager implementing the security module interface?
| // TODO: calling this should be restricted to...who? | ||
| function setSecurityModule(IACP99SecurityModule _securityModule) external { | ||
| securityModule = _securityModule; | ||
| } |
There was a problem hiding this comment.
Should the security module itself control its own upgrades? If not, should there be a SecurityUpdateModule?
| bytes calldata args | ||
| ) external returns (bytes32){ | ||
| bytes32 validationID = _initializeValidatorRegistration(input, weight); | ||
| securityModule.handleInitializeValidatorRegistration(validationID, weight, args); |
There was a problem hiding this comment.
I definitely need to go through the ACP again, but I would expect the security module to do some form of validation on the inputs before calling the _initializeValidatorRegistration. Or does it not matter since the security module will just revert anyway?
There was a problem hiding this comment.
It's a little tricky to reverse the order in our current implementation since the security module needs the validationID, which is provided by the manager contract. It will revert if the security module's checks fail, regardless of the ordering. We should definitely keep in mind patterns such as checks-effects-interactions if/when we decide to move forward on this implementation.
cam-schultz
changed the title
cam-schultz
changed the title
2 participants
Why this should be merged
Experimental implementation of ACP-99 using composition. Concrete contracts implement either
IACP99SecurityModuleorIACP99ValidatorManager.Notably, the contract sizes decrease significantly.
ERC20TokenStakingManagergoes from right at the 24kB size limit down to 14.6kB.This is not functional as-is, and is meant to demonstrate the necessary refactoring to implement ACP-99 using composition
How this works
classDiagram class IACP99SecurityModule { handleInitializeValidatorRegistration() handleCompleteValidatorRegistration() handleInitializeEndValidation() handleCompleteEndValidation() handleInitializeValidatorWeightChange() handleCompleteValidatorWeightChange() } <<interface>> IACP99SecurityModule class IACP99ValidatorManager { initializeValidatorSet() initializeValidatorRegistration() completeValidatorRegistration() initializeEndValidation() completeEndValidation() initializeValidatorWeightChange() completeValidatorWeightChange() } class PoSValidatorManager <<abstract>> PoSValidatorManager class ERC20TokenStakingManager class NativeTokenStakingManager class PoAValidatorManager class ACP99ValidatorManager class ValidatorManager <<abstract>> ValidatorManager ACP99ValidatorManager *-- IACP99SecurityModule ValidatorManager <|-- ACP99ValidatorManager IACP99ValidatorManager <|-- ACP99ValidatorManager IACP99SecurityModule <|-- PoSValidatorManager PoSValidatorManager <|-- ERC20TokenStakingManager PoSValidatorManager <|-- NativeTokenStakingManager IACP99SecurityModule <|-- PoAValidatorManagerThis implementation deviates slightly from the ACP-99 spec as written in that the ValidatorManager is the only contract that the end user interacts with. To achieve this, the
handle*method signatures must be generic enough to cover all possible SecurityModule use cases, notably PoA vs PoS. This results in a greater-than-minimal IACP99SecurityModule interface, where some implementations won't use certain arguments (for example PoA applications won't needpayablemethods, but native token staking applications will).How this was tested
How is this documented